Skip to content

More modernization - #9089

Open
anderseknert wants to merge 1 commit into
open-policy-agent:mainfrom
anderseknert:gardening
Open

More modernization#9089
anderseknert wants to merge 1 commit into
open-policy-agent:mainfrom
anderseknert:gardening

Conversation

@anderseknert

Copy link
Copy Markdown
Member

And allocation Pokémons caught. Gotta catch 'em all!

  • Add VarSet.DeleteFunc which largerly replaces the VarSet.Diff function, just diffing in place rather than by creating a new VarSet — one which we almost never used more than temporarily
  • Use unknownRef in eval over unknown where possible as Ref -> any boxing always allocates
  • Get rid of a few more custom comparison functions in favor of those in the stdlib..
  • ..and some more code replaced with stdlib/our helpers to reduce lines of code and help with readability
  • Fixed a few random heap allocations

And allocation Pokémons caught. Gotta catch 'em all!

- Add `VarSet.DeleteFunc` which largerly replaces the
  `VarSet.Diff` function, just diffing in place rather
  than by creating a new `VarSet` — one which we almost
  never used more than temporarily
- Use `unknownRef` in eval over `unknown` where possible
  as `Ref` -> any boxing always allocates
- Get rid of a few more custom comparison functions
  in favor of those in the stdlib..
- ..and some more code replaced with stdlib/our helpers
  to reduce lines of code and help with readability
- Fixed a few random heap allocations

Signed-off-by: Anders Eknert <anders.eknert@apple.com>

@srenatus srenatus left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Curious if you ran this through benchmarks of any sort. benchlab is handy for this, I think.

Comment thread v1/ast/term.go
// implementations to *Term in util.Map, util.MapKeys, etc.
func ToTerm[T Value](v T) *Term {
return NewTerm(v)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we need/want an interning variant?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not with my laptop today, but I think InternedTerm should work as it’s already generic. I should add something to the comment here though as that’s preferable for anything that may be interned.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh no, sorry, this one takes Value types rather than primitives. Not sure how we’d best take a Value and check if there’s an interned term containing that 🤔 But yeah, that would certainly be nice if we could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants